Proposal for inject/eject CD-ROM #369
Conversation
kubevirt-bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mhenriks
force-pushed
the
inject-eject-cdrom
branch
from
3c9c0e0 to
825f015
Compare
| kubevirt/kubevirt | ||
|
|
||
| # Design | ||
| Like most new features in KubeVirt, the building blocks already exist, just have to wire everything together |
There was a problem hiding this comment.
Please consider alternative designs and their pros/cons. In particular, can you consider following https://github.com/kubevirt/community/blob/e4645eb/design-proposals/vm-rollout-strategy/vm-rollout-strategy.md to enable a simpler, declarative, API. According to it, the user expresses a VM spec that includes a CDROM with a specific PVC and the VM controller makes that happen.
There was a problem hiding this comment.
I can address that in the design, but just to answer you inline, I think the functionality you describe can be generalized and included in an all encompassing "Declarative Volume Hotplug" design
There was a problem hiding this comment.
I can address that in the design,
Thanks.
I think the functionality you describe can be generalized and included in an all encompassing "Declarative Volume Hotplug" design
Right, I'd love to see that. But I'd be satisfied in seeing CDROM-only as a first step.
There was a problem hiding this comment.
I think just CD-ROM being declarative would be confusing to users.
To me, what is proposed here is a good first step in making this long awaited functionality available to users soon. Going full declarative is a lot more complicated. But is something I do agree that we should tackle eventually
There was a problem hiding this comment.
Please consider pros and cons each of these options in the text of the design.
There was a problem hiding this comment.
Adding my 2 cents here, declarative is a nice to have, however, this type of installation operation is imperative and kind of "manual". Do we really expect this to be used with git ops?
|
this would be great. this is a real user missing feature moving from alternatives. |
|
I would add an important non-goal about "config volumes", typically for service accounts. |
The fact that they they are isos is more of an implementation detail to me. Do end users really know/care when it is exposed as a disk? I state as a non-goal |
They do know, since they are directly consuming them. And they probably do care, since that limitation prevents service account data from getting updated.
You're right that's probably enough, thanks! |
|
|
||
| ## Goals | ||
| - Extend existing APIs to support inject/eject CD-ROM operations | ||
| - Extend virtctl to have cdrom specific commands |
There was a problem hiding this comment.
Is there a way to do this using only kubectl. In general we've been trying to avoid adding things to virtctl that can already be done entirely in kubectl.
There was a problem hiding this comment.
That would be declarative hotplug, which again, I think we should do eventually.
Users are currently used to adding/removing disks in a non-declarative way via virtctl. Also, there explicit requirements from customers to have a non declarative way to inject/eject CD-ROM.
|
/hold A request was made to compare pros/cons of doing just CD-ROM vs a more generalized API. This hold can be removed once that's addressed. |
kubevirt-bot
added
the
do-not-merge/hold
|
@mhenriks aren't we missing the setting of the boot order for the other disks? I mean, if we eject the cdrom, how does the VM know from which disk it has to boot the next time it is restarted? |
@stu-gott @dankenigsberg I don't think that it makes sense to debate the pros and cons of declarative vs subresource APIs in this design. Declarative volume hotplug is bigger topic, not just for CD-ROMs, and will be a separate design proposal. This proposal only describes two changes to existing APIs which are specific to inject/eject CD-ROM support:
That's it. Those two changes allow us to support inject/eject CD-ROM in short order by leveraging the existing hotplug machinery. How about we judge this proposal based on its merits and with the understanding that there will be a declarative implementation eventually? I kindly ask that the hold on this PR be removed. |
Hey @alicefr qemu seems to handle an empty cd-rom with |
@mhenriks I agree with you that the cost of implementation is an important consideration when choosing a course of action, but it cannot be the only one. I believe that there's a lot of sense, of having an API proposal consider alternatives, describing the benefits and costs of each, and finally choosing one. |
@dankenigsberg with regards to declarative vs subresource I don't see why we have to choose one. The subresource case is described here. The declarative case will be handled implicitly by declarative volume hotplug |
Signed-off-by: Michael Henriksen <[email protected]>
mhenriks
force-pushed
the
inject-eject-cdrom
branch
from
825f015 to
7624445
Compare
|
Inspired by feedback from the passionate CD-ROMers in the community and the k8s kep template to add |
| # Alternatives | ||
|
|
||
| ## Declatative API | ||
| Given the drawbacks of the subresource API, why not propose a declarative way to inject/eject CD-ROMs in this document? The advantages of a declarative API are obvious. It is "the Kubernetes way." Here's why: |
There was a problem hiding this comment.
Thanks @mhenriks. I appreciate that you've added this section.
However, I am not sure it is obvious for everyone why declarative APIs are so much better. For example, I'm not sure it was obvious to us when we devised the disk hotplug API, or the stop/start subresources. Would you please spell out a few of the reasons? From the tip of my keyboard I can think of:
- It is GitOps-friendly
- It is easier to integrate with exiting GUIs amd CLIs, which know how to patch a VM's CDROM section, but would have to learn to use a new subresource in order to modify a VM that happens to be running.
- It can be made race free. With the suggested API, a client that wants to eject the CDROM has to check if the VM is running and choose whether to patch the VM spec or use the subresource accordingly. During this period, the VM may have been stopped/crashed/lost connectivity.
Would you agree to add points in this line to this section?
There was a problem hiding this comment.
I truly apologize! you already have this written most of this in https://github.com/kubevirt/community/pull/369/files#diff-1eb0bbc806ec90076d40390805959b650e939e77e432f9a149b28736a80e34bbR180-R184
| Given the drawbacks of the subresource API, why not propose a declarative way to inject/eject CD-ROMs in this document? The advantages of a declarative API are obvious. It is "the Kubernetes way." Here's why: | ||
|
|
||
| ### It's bigger than just CD-ROM | ||
| Declarative inject/eject CD-ROM falls under the umbrella of Declarative Volume Hotplug. CD-ROMs are just one of the four [disk types](https://kubevirt.io/user-guide/storage/disks_and_volumes/#disks) that KubeVirt supports. The work required for supporting one disk type is roughly equal to supporting all four, requires no special considerations for CD-ROMs, and is worthy it's own design proposal. Ultimately we should support both declarative and subresource. Supporting just subresource initially would allow us to release this feature sooner. But that is not the only reason to support subresource. |
There was a problem hiding this comment.
I see. I hoped that we can start CD-ROM afresh with a good declarative API. Would you share a bit of history? Since CD-ROMs are just another disk, why did we not consider them in the original disk hotplug API?
To me, changing a CDROM seems closer to storage migration than adding a new disk. Implementation-wise, maybe we can piggy-back on top of the new storage migration API (which is declarative) rather than on the disk hotplug API?
| Declarative inject/eject CD-ROM falls under the umbrella of Declarative Volume Hotplug. CD-ROMs are just one of the four [disk types](https://kubevirt.io/user-guide/storage/disks_and_volumes/#disks) that KubeVirt supports. The work required for supporting one disk type is roughly equal to supporting all four, requires no special considerations for CD-ROMs, and is worthy it's own design proposal. Ultimately we should support both declarative and subresource. Supporting just subresource initially would allow us to release this feature sooner. But that is not the only reason to support subresource. | ||
|
|
||
| ### VirtualMachineInstances | ||
| If declarative is the only way to go for inject/eject, `VirtualMachineInstances` will be neglected. They can stand on their own without a `VirtualMachine` owner and currently support Subresource Volume Hotplug. VMIs are also immutable to regular users and we have no intention of changing that. |
There was a problem hiding this comment.
I don't believe I heard users requesting to modify the CDROM of volatile VirtualMachineInstance. "Orphan" VMIs are typically for short-lived tasks. CDROM eject is tyoically needed for long-living traditional VMs.
But let us assume this use case is important. I am not sure I understand the statement about immutability of VMI, though. The proposed API does mutate VMI, albeit via a subresource. Can you explain why it ok to modify a VMI in one way but not the other?
| If declarative is the only way to go for inject/eject, `VirtualMachineInstances` will be neglected. They can stand on their own without a `VirtualMachine` owner and currently support Subresource Volume Hotplug. VMIs are also immutable to regular users and we have no intention of changing that. | ||
|
|
||
| ### RBAC Limitations | ||
| Say you want to give a user permission to inject/eject CD-ROMs but not allow them to hotplug memory/cpu. You can do that with a subresource API. There is no straightforward way to do this with a declarative API. As more users come to KubeVirt from traditional virtualization environments with complex authorization requirements and no desire for GitOps, we may start looking at subresources a little more fondly. |
There was a problem hiding this comment.
That's a good point. I can think of a use case where a VM user is not allowed to modify the spec, but only the media that is inside the CDROM.
However, this kind of user should be allowed to change the CDROM also when the VM is not running; this kind of user should not be able to add more CDROMs or more disks. Does the proposed API intend to support these points?
What this PR does / why we need it:
Currently, a VM has to be rebooted to change the CD-ROM media. Let's leverage the hotplug volume API to fix that gaping hole in functionality
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: